Better workaround for issue #1187 (warning 40 due to @@deriving sexp) #1188
Conversation
|
I'm planning to find a better solution for #1187 eventually (e.g., looking into the generated code in detail and reporting a bug with the |
lens/column.ml
Outdated
| (* Workaround for issue #1187 (i.e., the @@deriving sexp clause on t | ||
| below creates code triggering warning 40): | ||
| We disable warning 40 within this module, and immediately include it. *) | ||
|
|
||
| [@@@ocaml.warning "-40"] |
There was a problem hiding this comment.
A better approach is to open the modules in the scope, i.e.
| (* Workaround for issue #1187 (i.e., the @@deriving sexp clause on t | |
| below creates code triggering warning 40): | |
| We disable warning 40 within this module, and immediately include it. *) | |
| [@@@ocaml.warning "-40"] | |
| open Sexplib0.Sexp_conv_record.Fields | |
| open Sexplib0.Sexp_conv_record.Kind |
This is more robust as it doesn't depend on the OCaml compiler's implicit resolution.
There was a problem hiding this comment.
These particular module paths are not visible from the use sites. I don't fully understand the module structure that Sexplib and Sexplib0 export, but I've now open-ed Sexplib0.Sexp_conv in all the relevant (toplevel) modules instead, and gotten rid of the include struct ... end.
frank-emrich
force-pushed
the
ppx_sexp_conv-workaround
branch
from
6d38f7a to
7ab3429
Compare
This PR implements a better workaround for #1187.
The previous workaround was to force using an older version of the
ppx_sexp_conv, which did not exhibit issue #1187. As such a version constraint may inconvenience users, this PR removes the version constraint and touches the problematic uses of@@deriving sexpdirectly.Unforutnately, locally suppressing the warning code 40 triggered by the code generated for
@@deriving sexpis somewhat tricky, as there is no AST node in the original source file to attach anocaml.warningattribute to.This PR circumvents this problem by wrapping the affected type definitions in an anonymous module, disabling warning 40 within that whole module, and then immediately
includeing said module.This limits the scope in which we actually disable warning 40 to exactly where it is needed.